Skip to content

Fix/stream seek negative position - #23907

Open
marc-mabe wants to merge 4 commits into
php:PHP-8.6from
marc-mabe:fix/stream-seek-negative-position
Open

marc-mabe wants to merge 4 commits into
php:PHP-8.6from
marc-mabe:fix/stream-seek-negative-position

Conversation

@marc-mabe

Copy link
Copy Markdown
Contributor

fixes #23905

This is targeting 8.6 even if this bug exists on 8.4 as well but #21433 conflicts and targets 8.6.
Please tell me if I should target PHP-8.4 instead.

@marc-mabe

Copy link
Copy Markdown
Contributor Author

Ping @iliaal @Girgias @ndossche as you where working/reviewing #21433

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, returning -1 is enough without setting *newoffs.
We had a similar issue in zlib before: 2709ebc

@marc-mabe

Copy link
Copy Markdown
Contributor Author

@ndossche I created to deeper test script comparing different streams and behavior (see #23905 (comment)).
Id I take POSIX as the source of truth this PR is still missing some cases -> will need to improve it.

@marc-mabe
marc-mabe marked this pull request as draft October 1, 2026 19:58
A failed seek reset the internal position to 0 but reported -1 as the
stream position, so ftell() returned false and a following SEEK_CUR
tripped an assertion. Leave the position unchanged instead, like plain
files.

php://temp was affected too while its data is held in memory, as it
forwards seeks to an inner php://memory stream. Once spilled to a
temporary file it already behaved correctly. Its seek no longer
reports -1 either when it has no inner stream.
A failed seek discarded the read buffer although the stream did not
move. A buffered stream has usually read ahead, so a following read or
write continued from where the stream had read ahead to, while ftell()
still reported the old position. Return early instead, keeping the
position, the read buffer and the filter state.
A failed seek clamped the internal position to 0 or the blob size but
reported -1 as the stream position, so ftell() returned false and a
following SEEK_CUR tripped an assertion. Leave both positions unchanged
instead, and reject a negative SEEK_SET offset explicitly rather than
relying on the size_t cast.
A failed seek clamped the internal position to 0 or the blob size but
reported -1 as the stream position, so ftell() returned false and a
following SEEK_CUR tripped an assertion. Leave both positions unchanged
instead, and reject a negative SEEK_SET offset explicitly rather than
relying on the size_t cast.
@marc-mabe
marc-mabe force-pushed the fix/stream-seek-negative-position branch from 0b8091a to bc78826 Compare October 2, 2026 13:39
@marc-mabe

Copy link
Copy Markdown
Contributor Author

@ndossche I reworked the branch and updated it to latest PHP-8.6.

What has been changed since last?

Fix failed seek position on php://memory

  • The failure branches no longer write the new offset at all, so both the internal and the reported position stay untouched.
  • php://temp is affected as well while its data is still in memory, as it forwards seeks to an inner php://memory stream. NEWS and the test title now mention it.
  • The test now covers php://memory, php://temp, php://temp/maxmemory:0 and the same three behind php://filter/string.rot13/..., each with SEEK_SET, SEEK_CUR and SEEK_END before the start and past the end.
  • Removed the redundant eof/fatal_error resets from the memory seek handler, as php_stream_seek() already does that on success. php_stream_temp_seek() no longer reports -1 when it has no inner stream.

New commit: Fix failed seek discarding the read buffer
While extending the blob tests I found that php_stream_seek() discarded the read buffer even when the seek failed. A buffered stream has usually read ahead, so after a failed seek the next read skipped the buffered data and a write landed where the stream had read ahead to, while ftell() still reported the old position. This also affects plain files and user stream wrappers:

$fp = fopen($file, 'r+');    // "hello world"
fread($fp, 5);               // "hello"
fseek($fp, -12, SEEK_END);   // -1, ftell() is still 5
fread($fp, 5);               // before: "", now: " worl"

A failed seek now returns early and keeps the position, the read buffer and the filter state. The new test ext/standard/tests/streams/stream_seek_failure_keeps_buffer.phpt covers a plain file and a user stream wrapper.

Fix failed seek position on SQLite3 / PDO SQLite blob streams

  • The failure branches no longer write the new offset. Before, after a partial (buffered) read, a failed seek still moved ftell() to the position the blob had read ahead to.
  • A negative SEEK_SET offset is now rejected explicitly instead of relying on the size_t cast.
  • The tests now cover read-only and read-write blobs, all whence values before the start and past the end, failed seeks after a partial read, and a write after a failed seek past the end. That write now happens at the unchanged position instead of failing with "It is not possible to increase the size of a BLOB".
    • This is because the failed seek before does not change the position anymore. Writing still works until the end.

Not addressed / possible follow-ups
These came up while analysing seek behaviour across stream types. They are out of scope for this PR, so I left them for follow-ups:

  • Assertion failure _php_stream_seek at streams/streams.c #21700 looks related, as it hits the same stream->position >= 0 assertion, but it isn't fixed by this PR. The cause is different: php_stream_seek() converts SEEK_CUR into an absolute SEEK_SET target without checking that the target isn't negative (only an explicit SEEK_SET is checked). So a user wrapper receives SEEK_SET -1, accepts it, and stream_tell() then reports -1. A possible follow-up is to reject negative SEEK_CUR targets in php_stream_seek().
  • Phar refuses seeks past the end of an entry without a warning, while POSIX allows them, and a write then fills the gap with zeros. A following read or write correctly happens at the unchanged position. Allowing the seek would require phar_stream_read() to stop at the end of the entry. It currently relies on the seek refusal: with a position past the end, uncompressed_filesize - position underflows. It also computes EOF with ==, which sets EOF early after a read that ends exactly at the end of the entry.
  • zlib: gzseek() doesn't support SEEK_END. In write mode, backward seeks are impossible, and a forward seek past the end writes the zeros straight away rather than on the next write. These are mostly zlib limitations.
  • zip: php_zip_ops_seek() still sets the new offset from zip_ftell() when zip_fseek() fails. Now that a failed seek keeps the read buffer, this would let the position jump ahead while the buffered data is kept, at least with a libzip where entries are seekable. I couldn't reproduce it locally because zip entries weren't seekable there, so the seek is emulated.
  • No error reporting: apart from zlib's SEEK_END, a failed seek returns -1 without a warning or reason.

@marc-mabe
marc-mabe marked this pull request as ready for review October 2, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants